Fix phpstan/phpstan#14211: incorrect type inference with dependent types#5092
Open
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
Open
Fix phpstan/phpstan#14211: incorrect type inference with dependent types#5092phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
phpstan-bot wants to merge 1 commit intophpstan:2.1.xfrom
Conversation
- Fixed conditional expression resolution in filterBySpecifiedTypes to intersect types instead of overwriting when multiple conditional expressions fire for the same target - When two conditionals with the same guard produced different types (one more specific than the other), the less specific type could overwrite specifiedExpressions, causing cascading wrong matches that led to variables being incorrectly narrowed to *NEVER* - New regression test in tests/PHPStan/Analyser/nsrt/bug-14211.php Closes phpstan/phpstan#14211
staabm
changed the title
staabm
approved these changes
Feb 28, 2026
| if (array_key_exists($conditionalExprString, $specifiedExpressions) && $specifiedExpressions[$conditionalExprString]->getCertainty()->yes() && $newTypeHolder->getCertainty()->yes()) { | ||
| $specifiedExpressions[$conditionalExprString] = ExpressionTypeHolder::createYes( | ||
| $newTypeHolder->getExpr(), | ||
| TypeCombinator::intersect($specifiedExpressions[$conditionalExprString]->getType(), $newTypeHolder->getType()), |
Contributor
There was a problem hiding this comment.
This might add a lot of possibly useless intersect calls.
For instance if the next newTypeHolder does not have a certainty of Yes. we will override the value.
Might be better to check it first by computing all the newTypeHolder and
- if they all have a yes certainty we intersect them
- if not, we take the last one
WDYT @staabm ?
| $conditions[$conditionalExprString][] = $conditionalExpression; | ||
| $specifiedExpressions[$conditionalExprString] = $conditionalExpression->getTypeHolder(); | ||
| $newTypeHolder = $conditionalExpression->getTypeHolder(); | ||
| if (array_key_exists($conditionalExprString, $specifiedExpressions) && $specifiedExpressions[$conditionalExprString]->getCertainty()->yes() && $newTypeHolder->getCertainty()->yes()) { |
Contributor
There was a problem hiding this comment.
It's unclear to me why we're only concerned about Yes & Yes.
Looking at this code, the last newTypeHolder override all the previous one if there is some Maybe certainty in the game.
Is this condition clear to you @staabm ?
Contributor
There was a problem hiding this comment.
Updating the code to
if (array_key_exists($conditionalExprString, $specifiedExpressions)) {
does not creates any failure. I feel like we're lacking of coverage. But I never worked with ExpressionTypeHolder...
| $specifiedExpressions[$conditionalExprString] = $conditionalExpression->getTypeHolder(); | ||
| $newTypeHolder = $conditionalExpression->getTypeHolder(); | ||
| if (array_key_exists($conditionalExprString, $specifiedExpressions) && $specifiedExpressions[$conditionalExprString]->getCertainty()->yes() && $newTypeHolder->getCertainty()->yes()) { | ||
| $specifiedExpressions[$conditionalExprString] = ExpressionTypeHolder::createYes( |
Contributor
There was a problem hiding this comment.
ExpressionTypeHolder has a method and
public function and(self $other): self
{
if ($this->type === $other->type || $this->type->equals($other->type)) {
if ($this->certainty->and($other->certainty)->yes()) {
return $this;
}
if ($this->certainty->maybe()) {
return $this;
}
return $other;
}
return new self(
$this->expr,
TypeCombinator::union($this->type, $other->type),
$this->certainty->and($other->certainty),
);
}
Should we introduce a new method for ExpressionTypeHolder which does the intersect ?
| $conditions[$conditionalExprString][] = $conditionalExpression; | ||
| $specifiedExpressions[$conditionalExprString] = $conditionalExpression->getTypeHolder(); | ||
| $newTypeHolder = $conditionalExpression->getTypeHolder(); | ||
| if (array_key_exists($conditionalExprString, $specifiedExpressions) && $specifiedExpressions[$conditionalExprString]->getCertainty()->yes() && $newTypeHolder->getCertainty()->yes()) { |
Contributor
There was a problem hiding this comment.
Updating the code to
if (array_key_exists($conditionalExprString, $specifiedExpressions)) {
does not creates any failure. I feel like we're lacking of coverage. But I never worked with ExpressionTypeHolder...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a regression introduced by #5048 (commit c5bfadb) where a boolean variable assigned from
isset()would incorrectly be narrowed to*NEVER*on the secondifcheck within the same function.The issue manifested when:
isset($data['x'])$m = isset($data['y'])was assignedif ($m)correctly narrowed$mtotrueif ($m)incorrectly narrowed$mto*NEVER*Changes
src/Analyser/MutatingScope.php(filterBySpecifiedTypesmethod) to intersect types when multiple conditional expressions fire for the same target expression, instead of overwriting with the last matchtests/PHPStan/Analyser/nsrt/bug-14211.phpRoot cause
When
$m = isset($data['y'])is processed, PHPStan creates conditional expressions for the dependent types. Two conditionals are created for$datawith the same guard{$m → true}:processSureTypesForConditionalExpressionsAfterAssign:$datawithhasOffset('y')(correct, more specific)processSureNotTypesForConditionalExpressionsAfterAssign:$datawithouthasOffset('y')(from removingnullfrom a non-nullable array type — effectively a no-op)When both conditionals fired in the resolution loop,
specifiedExpressions[$data]was overwritten by the last match (the less specific type withouthasOffset('y')). This caused a cascading failure:$data= type-without-y,$data['y']= unset" matched incorrectly$data= type-without-y,$m= false" also matchedtrueandfalsefor$mproduced*NEVER*The fix intersects the types when accumulating
specifiedExpressionsfrom multiple matching conditionals (when both haveYescertainty), ensuring the most specific type is used for subsequent guard matching.Test
The regression test
tests/PHPStan/Analyser/nsrt/bug-14211.phpreproduces the exact scenario from the issue: anissetguard followed by a boolean assignment from anotherisset, with two consecutiveifchecks on the boolean variable. It verifies that the secondif ($m)correctly narrows$mtotrue(not*NEVER*).Fixes phpstan/phpstan#14211